Skip to content

Convert Go e2e test to Python Test to leverage SDK directly #451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Feb 7, 2024
Merged

Convert Go e2e test to Python Test to leverage SDK directly #451

merged 10 commits into from
Feb 7, 2024

Conversation

ChristianZaccaria
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria commented Jan 30, 2024 •
edited
Loading

Issue link

Jira: https://issues.redhat.com/browse/RHOAIENG-2058

What changes have been made

  • Converted Go e2e test to a Python test to leverage the SDK directly.
  • Removed unnecessary RBAC, ConfigMap, and Job specs.
  • Removed Python script and moved its contents directly into the test file, this way allowing to perform assertions in the test at any given point.
  • Adjusted ingress_options in the script to include this annotation: nginx.ingress.kubernetes.io/proxy-body-size: "100M", else, on KinD, we would encounter the following error:
E       RuntimeError: Request failed with status code 413: <html>
E       <head><title>413 Request Entity Too Large</title></head>
  • Adjusted e2e workflow + added sdk-user with limited permissions on KinD cluster to run the e2e tests.

Verification steps

To run the e2e test locally on KinD cluster, follow these steps:

Note: If not already done so, please add in your local /etc/hosts file 127.0.0.1 kind. This will map your localhost IP address to the KinD cluster's hostname. This is already performed on GitHub Actions

  • Setup Phase: 
    make kind-e2e
export CLUSTER_HOSTNAME=kind
export CODEFLARE_TEST_TIMEOUT_LONG=20m
make deploy -e IMG=quay.io/project-codeflare/codeflare-operator:v1.1.0
make setup-e2e
  • Test Phase:
    • Once we have the codeflare-operator and kuberay-operator running and ready, we can run the e2e test on the codeflare-sdk repo:
    poetry install --with test,docs
poetry run pytest -v -s ./tests/e2e/mnist_raycluster_sdk_test.py

To run the e2e test locally on OpenShift, follow these steps:

  • Setup Phase: 
    make deploy -e IMG=quay.io/project-codeflare/codeflare-operator:v1.1.0
make setup-e2e
  • Test Phase:
    • Once we have the codeflare-operator and kuberay-operator running and ready, we can run the e2e test on the codeflare-sdk repo:
    poetry install --with test,docs
poetry run pytest -v -s ./tests/e2e/mnist_raycluster_sdk_test.py

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@openshift-ci openshift-ci bot requested review from dimakis and Maxusmusti January 30, 2024 17:56
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2024
sutaakar
sutaakar reviewed Jan 31, 2024
View reviewed changes
@ChristianZaccaria ChristianZaccaria force-pushed the python-test branch 2 times, most recently from a16887b to 61a01df Compare January 31, 2024 15:28
@ChristianZaccaria ChristianZaccaria mentioned this pull request Jan 31, 2024
Closed
4 tasks
Bobbins228
Bobbins228 reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2024
sutaakar
sutaakar reviewed Feb 1, 2024
View reviewed changes
Copy link
Contributor

@sutaakar sutaakar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2024
@VanillaSpoon
Copy link
Contributor

This is great :) Ran the tests locally also and they run through as expected
\lgtm

@Srihari1192 Srihari1192 mentioned this pull request Feb 2, 2024
Merged
4 tasks
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2024
@ChristianZaccaria
Copy link
Collaborator Author

I mistakenly pushed to this branch by accident, reverted changes. No new changes have been made - good to merge.

astefanutti
astefanutti reviewed Feb 5, 2024
View reviewed changes
Copy link
Contributor

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What user is impersonated to run the tests? It seems it's using the default user that's cluster admin, in which case that's a regression from the existing e2e tests?

@sutaakar
Copy link
Contributor

sutaakar commented Feb 5, 2024

project-codeflare/codeflare-common#33 is adding support for custom user. That can be used to specify user with specific allowed roles.
For usage see project-codeflare/codeflare-operator#462

@ChristianZaccaria
Copy link
Collaborator Author

@astefanutti @sutaakar , I was told to keep the issues separated where after merging we would add the unprivileged user to run the test: https://issues.redhat.com/browse/RHOAIENG-60

We could add it in the scope of this PR too if preferred.

@ChristianZaccaria
Copy link
Collaborator Author

I added in the e2e workflow an sdk-user with limited RBAC permissions who runs the e2e tests. This is currently only performed in the e2e test workflow. For local testing, should I add in the test a script that creates the sdk-user + permissions if it doesn't already exist in the cluster, or should we use the cluster admin for local tests? Thanks @sutaakar @astefanutti

@astefanutti
Copy link
Contributor

I added in the e2e workflow an sdk-user with limited RBAC permissions who runs the e2e tests. This is currently only performed in the e2e test workflow. For local testing, should I add in the test a script that creates the sdk-user + permissions if it doesn't already exist in the cluster, or should we use the cluster admin for local tests? Thanks @sutaakar @astefanutti

For local testing, I'd avoid mixing provisioning and testing logic, so an option would be to document how to setup the cluster (even specific to KinD would be fine) and run the tests with the correct user.

@astefanutti
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024
@ChristianZaccaria
Copy link
Collaborator Author

I added in the e2e workflow an sdk-user with limited RBAC permissions who runs the e2e tests. This is currently only performed in the e2e test workflow. For local testing, should I add in the test a script that creates the sdk-user + permissions if it doesn't already exist in the cluster, or should we use the cluster admin for local tests? Thanks @sutaakar @astefanutti

For local testing, I'd avoid mixing provisioning and testing logic, so an option would be to document how to setup the cluster (even specific to KinD would be fine) and run the tests with the correct user.

That makes sense, thanks Antonin! I added e2e documentation there now and updated the readme.

sutaakar
sutaakar reviewed Feb 7, 2024
View reviewed changes
Copy link
Contributor

@sutaakar sutaakar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024
@astefanutti
Copy link
Contributor

/approve

Great job!

Bobbins228
Bobbins228 approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti, Bobbins228

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 660f328 into project-codeflare:main Feb 7, 2024
@Bobbins228 Bobbins228 mentioned this pull request Mar 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sutaakar sutaakar sutaakar left review comments

@Bobbins228 Bobbins228 Bobbins228 approved these changes

@dimakis dimakis Awaiting requested review from dimakis

@Maxusmusti Maxusmusti Awaiting requested review from Maxusmusti

@astefanutti astefanutti Awaiting requested review from astefanutti

Assignees

@astefanutti astefanutti

@sutaakar sutaakar

@Bobbins228 Bobbins228

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ChristianZaccaria @VanillaSpoon @sutaakar @astefanutti @Bobbins228 @openshift-merge-robot